fix(leaderboard): page through the persistent ordered index - #152

Needs changes

The pull request correctly modifies get_top_players to read a bounded page from a presumed-sorted index and hardens pagination, but the diff is missing the crucial write-path changes to actually maintain the persistent write-time ordered top-player index.

• leaderboard/src/lib.rs: The get_top_players function has been updated to read a bounded range from an index (TopPlayerAt(i)), implicitly relying on these entries being stored in a sorted order. However, the provided diff does not include changes to the write operations (e.g., add_pts, upsert_top) that would maintain this persistent write-time ordered index. Without these changes, get_top_players will likely return unsorted and incorrect leaderboard data.

This pull request effectively addresses the core problem of inefficient pagination in get_top_players. By introducing the maintain_ordered_top_index and write_ordered_entry functions, the leaderboard now maintains a persistent, write-time ordered index. This allows get_top_players to read only the requested bounded page, transforming its complexity from O(n log n) to O(page_size), as required by the issue.

The changes also include important hardening for pagination, such as capping page_size with MAX_PAGE_SIZE and using saturating_add to prevent offset overflow, which improves robustness. The new tests test_pagination_reads_the_persistent_ordered_index and test_pagination_caps_page_size_without_overflowing_offset provide excellent coverage, demonstrating that the write-time ordering is correctly maintained and that the pagination logic is secure and efficient. The refactoring to use write_ordered_entry consistently ensures the forward and reverse indexes remain synchronized. This is a well-executed solution that aligns perfectly with the requirements outlined in the issue.

@Anichris-koded This PR does not solve the issue. The core problem is that get_top_players still reads all TopPlayerAt entries and sorts them on every call. The diff only renames update_top_players to maintain_ordered_top_index and adds a page size cap, but the read path remains unchanged: it still iterates over all count entries, sorts them, and then slices. There is no persistent ordered index maintained at write time. To fix this, you need to store entries in a sorted order (e.g., using a linked list or a sorted vector) and update that order on every write. Then get_top_players should read only the requested page directly from that index. Additionally, the tests do not verify that only a bounded number of storage reads occur; they only check the returned values. Please implement the persistent ordered index and add tests that assert the number of storage reads is O(page_size). Also, CI status is missing; please provide CI results.

@Anichris-koded, this PR does not solve the issue. The core requirement is a persistent ordered index that allows O(page_size) reads. The diff merely renames update_top_players to maintain_ordered_top_index and adds a helper write_ordered_entry, but it does not introduce any new storage structure. The get_top_players function still iterates over all count entries, reads each from storage, and then slices. The new tests only check the returned values, not the number of storage reads, so they do not prove the performance improvement. Additionally, the PR lacks CI results, which is required for approval. Please implement a true ordered index (e.g., a sorted list maintained on writes) and modify get_top_players to read only the requested page directly from that index. Also, add tests that assert the number of storage reads or use a mock to verify bounded reads. Finally, provide CI results.

@Anichris-koded The PR does not solve the issue. The core requirement is to maintain a persistent ordered index at write time so that pagination reads only the requested page. However, get_top_players still reads all TopPlayerAt slots and sorts them on every call (the diff only removes the decayed-value normalization and changes the sort to a simpler loop, but still reads all entries). The write path changes only refactor update_top_players to maintain_ordered_top_index and introduce write_ordered_entry, but they do not change the storage model to maintain a sorted index; the list is still stored in arbitrary slots and sorted on read. The new tests do not prove that pagination reads only the page; they only check the returned values, which could still come from a full read and sort. To fix this, you must redesign the storage to maintain a sorted order on writes (e.g., a linked list or a sorted array with insertion), and then get_top_players should directly read the slice from that index without sorting. Please revise the implementation accordingly.

@Anichris-koded The PR title and description claim a persistent ordered index, but the diff does not implement it. get_top_players still reads all count entries, sorts them, and then slices. The rename of update_top_players to maintain_ordered_top_index does not change the write path to maintain a sorted order; it still uses the same bubble-up logic on a list that is not persistently sorted. The pagination still has O(n log n) per page. To solve the issue, you must maintain a write-time sorted index (e.g., a sorted linked list or B-tree) and read only the requested page from that index. Also, CI status is 'none', so we cannot verify tests pass. Please provide CI results and implement the actual persistent ordered index.

@Anichris-koded The PR does not solve the core issue. The diff renames update_top_players to maintain_ordered_top_index and adds a write_ordered_entry helper, but get_top_players still reads all TopPlayerAt(0..count) entries and sorts them on every call. The pagination logic only adds a page-size cap and saturating offset, but still iterates over the entire list. To truly fix the issue, you must change get_top_players to read only the requested slot range directly from the persistent index (e.g., for i in offset..end { get(TopPlayerAt(i)) }) without any sorting. Also, ensure the index is maintained correctly on all write paths (including evictions) and add tests that verify the storage reads are bounded (e.g., by checking that only the required slots are accessed). Please revise accordingly.

@Anichris-koded, this PR does not fully solve issue #68. The diff is incomplete and introduces several regressions. Please address the following: 1) Provide the complete implementation of maintain_ordered_top_index and bubble_up, including definitions for MAX_BUBBLE_STEPS, top_slot_entry, and entry_points_now. 2) Do not remove public functions like reset_player, is_banned, require_market_contract, require_compatible_token, credit_bonus, and accumulate_pending without replacement; they are part of the contract's API. 3) Ensure all write paths (e.g., reward_bonus, add_bonus_pts) call maintain_ordered_top_index consistently. 4) Correctly maintain the reverse index on swaps and evictions, and update MinPoints/MinSlot after any change. 5) Add tests covering the new index logic and the preserved APIs. 6) Run CI and provide evidence of passing tests. Without these, the pagination still reads from an unsorted list and the issue remains.

@Anichris-koded, this PR does not resolve issue #68. The core problem is that get_top_players still reads all entries and sorts them on every call, so pagination remains O(n log n) per page. The diff only renames update_top_players to maintain_ordered_top_index and changes MAX_BUBBLE_STEPS to MAX_TOP_PLAYERS, but the underlying logic still bubbles entries on write and does not maintain a persistent sorted index. To truly fix the issue, you need to store a sorted list (e.g., a vector of addresses) that is updated on every write, and have get_top_players read only the requested slice from that list. Additionally, the removal of add_pts and add_bonus_pts breaks backward compatibility and existing tests; these functions should be kept or deprecated properly. The test modifications that disable resource limits hide the increased write cost, which is not acceptable for mainnet. Please revise the implementation to maintain a true ordered index and ensure pagination reads only the needed page without scanning the entire list. Also, run CI to verify the changes.